Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up go modules. #68

Merged
merged 3 commits into from
Apr 12, 2019

Conversation

SpeedyCoder
Copy link
Contributor

No description provided.

@johanbrandhorst
Copy link
Owner

This looks great, thanks. What commands did you run to produce this? It's possible it's better simply because I haven't tried re-resolving the dependencies with Go 1.12.

@johanbrandhorst
Copy link
Owner

CircleCI seems to think we can do one better:

git diff --exit-code

diff --git a/go.mod b/go.mod
index bafee95..b2ab97b 100644
--- a/go.mod
+++ b/go.mod
@@ -38,7 +38,6 @@ require (
 	golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3
 	golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6
 	golang.org/x/time v0.0.0-20190308202827-9d24e82272b4 // indirect
-	google.golang.org/appengine v1.4.0 // indirect
 	google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107 // indirect
 	google.golang.org/grpc v1.20.0
 	gotest.tools v2.2.0+incompatible // indirect

https://circleci.com/gh/johanbrandhorst/certify/498

Honestly though, if this is hard to reproduce, I can just remove the vendor check. It's never worked consistently for me.

@johanbrandhorst
Copy link
Owner

Hm, the generate error is a little more sinister. I don't know if we can update moq, as you can see the latest version has produced a generation error:

diff --git a/issuers/aws/mocks/client.mock.go b/issuers/aws/mocks/client.mock.go
index ee8d6f3..a31c670 100644
--- a/issuers/aws/mocks/client.mock.go
+++ b/issuers/aws/mocks/client.mock.go
@@ -6,6 +6,7 @@ package mocks
 import (
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/acmpca"
+	"github.com/johanbrandhorst/certify/vendor/github.com/aws/aws-sdk-go-v2/service/acmpca/acmpcaiface"
 	"sync"
 )
 
@@ -35,6 +36,10 @@ var (
 	lockACMPCAAPIMockWaitUntilCertificateIssuedWithContext              sync.RWMutex
 )
 
+// Ensure, that ACMPCAAPIMock does implement ACMPCAAPI.
+// If this is not the case, regenerate this file with moq.
+var _ acmpcaiface.ACMPCAAPI = &ACMPCAAPIMock{}
+
 // ACMPCAAPIMock is a mock implementation of ACMPCAAPI.
 //
 //     func TestSomethingThatUsesACMPCAAPI(t *testing.T) {

Also, you'll need to regenerate the protofiles in the test with a newer version of protoc-gen-go.

@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   67.91%   67.91%           
=======================================
  Files           8        8           
  Lines         402      402           
=======================================
  Hits          273      273           
  Misses         81       81           
  Partials       48       48

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 871b8c2...02f1e15. Read the comment docs.

@SpeedyCoder
Copy link
Contributor Author

Goland seems to be sticking google.golang.org/appengine v1.4.0 // indirect to go.mod file for some reason 😅 Will look at the other issue 👍

@johanbrandhorst
Copy link
Owner

I think if we can keep the previous version of moq and regenerate the proto files with the new protoc-gen-go we should be good.

@johanbrandhorst
Copy link
Owner

Looks like the vendor job is working again

@SpeedyCoder
Copy link
Contributor Author

I think if we can keep the previous version of moq and regenerate the proto files with the new protoc-gen-go we should be good.

cool will do that

@johanbrandhorst
Copy link
Owner

Looks like running go generate adds the appengine dependency back... I really don't understand it, but at this point I wouldn't hate just to remove the vendor job from .circleci/config.yaml if you can't get it to work.

@johanbrandhorst
Copy link
Owner

Ah, you'll need to copy the environment variable for that job, it runs without implicit module support. I think it's because of moq. Sorry 😂.

@SpeedyCoder
Copy link
Contributor Author

cool everything seems to be happy now

Copy link
Owner

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for this cleanup :). I'll release v1.2.0 after this merge.

@johanbrandhorst johanbrandhorst merged commit bb3edb9 into johanbrandhorst:master Apr 12, 2019
@SpeedyCoder
Copy link
Contributor Author

awesome thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants